Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: persistent tx-pool data into a file when it has been shutdown #2656

Closed
wants to merge 5 commits into from

Conversation

quake
Copy link
Member

@quake quake commented Apr 20, 2021

base on #2621, using the TransactionVec struct to persistent tx-pool data directly without new schema defination.


This change is Reviewable

@nervos-bot-user

This comment has been minimized.

@stale

This comment has been minimized.

@stale stale bot added the s:wontfix Status: Wont fix label Jun 9, 2021
@quake quake removed the s:wontfix Status: Wont fix label Jun 10, 2021
Copy link
Member

@doitian doitian left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does user know where the file is and how to delete it? Consider add an option in ckb reset-data to help user delete the pool persistent file and ask user to use the command like ckb reset-data --pool to delete the corrupted file.

I modified the error message, it will print the file path:

TxPool Error: The tx-pool persisted data file ["/home/xxx/ckb/data/tx_pool_persisted_data.v1"] is broken, please delete it and restart, cause: TransactionVecReader total size doesn't match, expect 1717859076, actual 8

@doitian

This comment has been minimized.

test/src/specs/tx_pool/pool_persisted.rs Show resolved Hide resolved
tx-pool/src/service.rs Outdated Show resolved Hide resolved
@keroro520
Copy link
Contributor

When systemctl stop ckb-node-8114, it sends SIGTERM to stop the service, will SIGTERM trigger dumping the tx-pool?

tx-pool/src/pool.rs Outdated Show resolved Hide resolved
@doitian doitian added the s:waiting-on-author Status: The marked PR is awaiting some action (such as code changes) from the PR author. label Jul 30, 2021
@quake
Copy link
Member Author

quake commented Jul 30, 2021

When systemctl stop ckb-node-8114, it sends SIGTERM to stop the service, will SIGTERM trigger dumping the tx-pool?

Yes

@quake
Copy link
Member Author

quake commented Jul 30, 2021

What does generate_transaction() does? Why this transaction is special that we cannot create it in the loop?

the doc of generate_transaction():

// generate a transaction which spend tip block's cellbase and send it to pool through rpc.

@quake quake requested a review from a team as a code owner July 30, 2021 12:54
@quake
Copy link
Member Author

quake commented Jul 30, 2021

@doitian @keroro520 @zhangsoledad I have rebased this PR with develop branch and resolved issues accordingly, please review again, thanks.

@quake quake force-pushed the quake/tx-pool-persistent branch 3 times, most recently from 25f17d0 to 6471e11 Compare July 30, 2021 13:11
keroro520
keroro520 previously approved these changes Jul 30, 2021
@quake quake force-pushed the quake/tx-pool-persistent branch 2 times, most recently from d0ecfd6 to eb259be Compare July 30, 2021 14:22
@doitian doitian requested a review from keroro520 July 30, 2021 23:28
Copy link
Member

@doitian doitian left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor and Nit (Not important thing) mean the suggestions do not block the PR.

Reviewed 7 of 28 files at r3, 22 of 22 files at r4.
Reviewable status: all files reviewed, 10 unresolved discussions (waiting on @keroro520, @quake, and @zhangsoledad)


test/src/node.rs, line 30 at r4 (raw file):

struct ProcessGuard {
    pub child: Child,

Minor: Use Option<Child>


tx-pool/src/pool.rs, line 184 at r4 (raw file):

    }

    /// Add tx which proposed but still uncommittable to gap pool

Nit, add comment about the return value.


tx-pool/src/service.rs, line 407 at r4 (raw file):

    }

    /// send suspend chunk process cmd

Minor

-   /// send suspend chunk process cmd
+   /// Sends suspend chunk process cmd 

tx-pool/src/service.rs, line 423 at r4 (raw file):
Minor

    /// Saves tx pool into disk.

tx-pool/src/service.rs, line 440 at r4 (raw file):

    fn load_persisted_data(&self, mut txs: Vec<TransactionView>) -> Result<(), AnyError> {
        if !txs.is_empty() {

Minor, early return reduces indentation.


tx-pool/src/service.rs, line 442 at r4 (raw file):

        if !txs.is_empty() {
            info!("Loading persisted tx-pool data, total {} txs", txs.len());
            // a trick to commit transactions with the correct order

Minor, explain what's the trick:

In each loop, try to add all the remaining transactions. Remove the successfully added transactions.

Stop the loop until there's no transactions to add in an iteration.


tx-pool/src/service.rs, line 444 at r4 (raw file):

            // a trick to commit transactions with the correct order
            loop {
                let mut failed_txs = Vec::new();

We can use Vec::drain to leave only the failed txs. Just remember the vec size before drain. If the size does not change, early return the loop. The loop also can use while !txs.empty() instead.


tx-pool/src/service.rs, line 461 at r4 (raw file):

                    break;
                }
                txs = failed_txs;

FYI, when I first review the code, it confused me because the assignment is in the last of loop. I don't know it when I reviewed the two break checks. Renaming the variable txs to remaining_txs can help, but using while !txs.empty() is better to give the hint that we are slowly draining the vector.

Copy link
Member

@doitian doitian left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: all files reviewed, 10 unresolved discussions (waiting on @keroro520, @quake, and @zhangsoledad)


tx-pool/src/service.rs, line 423 at r4 (raw file):

Previously, doitian (ian) wrote…

Minor

    /// Saves tx pool into disk.

Keep consistent: saves -> Saves


tx-pool/src/service.rs, line 444 at r4 (raw file):

Previously, doitian (ian) wrote…

We can use Vec::drain to leave only the failed txs. Just remember the vec size before drain. If the size does not change, early return the loop. The loop also can use while !txs.empty() instead.

Sorry, it should beVec::retain

tx-pool/src/service.rs Outdated Show resolved Hide resolved
@quake
Copy link
Member Author

quake commented Aug 4, 2021

@doitian @zhangsoledad updated accordingly to your comments, please review again, thanks.

@quake quake force-pushed the quake/tx-pool-persistent branch 3 times, most recently from 6a17fd2 to e453fd4 Compare August 4, 2021 06:32
@doitian
Copy link
Member

doitian commented Aug 4, 2021

Will the dump be saved as JSON or Molecule binary?

@quake
Copy link
Member Author

quake commented Aug 4, 2021

Will the dump be saved as JSON or Molecule binary?

molecule binary

Copy link
Member

@doitian doitian left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 13 of 13 files at r5.
Reviewable status: all files reviewed, 8 unresolved discussions (waiting on @keroro520, @quake, and @zhangsoledad)


tx-pool/src/service.rs, line 443 at r5 (raw file):

        if !txs.is_empty() {
            info!("Loading persisted tx-pool data, total {} txs", txs.len());
            let mut failed_txs = 0;

Minor suggestion: Iter::all can simplify the code here.

@doitian doitian removed the s:waiting-on-author Status: The marked PR is awaiting some action (such as code changes) from the PR author. label Aug 7, 2021
@doitian
Copy link
Member

doitian commented Aug 7, 2021

@zhangsoledad @keroro520 please review

Copy link
Member

@doitian doitian left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hold to merge it locally.

Still waiting for another reviewer.

doitian added a commit that referenced this pull request Aug 10, 2021
@doitian
Copy link
Member

doitian commented Aug 10, 2021

Merged in cc665b3

@doitian doitian closed this Aug 10, 2021
@doitian doitian mentioned this pull request Aug 10, 2021
3 tasks
@doitian doitian deleted the quake/tx-pool-persistent branch August 11, 2021 00:16
@doitian doitian mentioned this pull request Sep 3, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants